Skip to content

ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar#9345

Closed
nealrichardson wants to merge 6 commits into
apache:masterfrom
nealrichardson:arrow-datum
Closed

ARROW-10089: [R] inject base class for Array, ChunkedArray and Scalar#9345
nealrichardson wants to merge 6 commits into
apache:masterfrom
nealrichardson:arrow-datum

Conversation

@nealrichardson

Copy link
Copy Markdown
Member

The impetus for this change was to get S3 Ops method dispatch to work for Array == Scalar. This is tested in the third commit.

@github-actions

Copy link
Copy Markdown

Comment thread r/R/table.R Outdated

@ianmcook ianmcook Jan 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a TODO/comment here describing why this has not yet been changed to a method of the base class ArrowTabular? And another one where $<-.Table is defined

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up moving them to ArrowTabular and left stop(TODO) messages in the missing RecordBatch methods.

@ianmcook

ianmcook commented Jan 29, 2021

Copy link
Copy Markdown
Member

@nealrichardson could you please confirm that this is correct?

  • RecordBatch and Table now inherit the synthetic class ArrowTabular
  • Array, ChunkedArray, and Scalar now inherit the synthetic class ArrowDatum
  • These synthetic classes do not exist in the C++ class hierarchy; they are used here because they simplify S3 method dispatch.

@nealrichardson

Copy link
Copy Markdown
Member Author

@nealrichardson could you please confirm that this is correct?

  • RecordBatch and Table now inherit the synthetic class ArrowTabular
  • Array, ChunkedArray, and Scalar now inherit the synthetic class ArrowDatum
  • These synthetic classes do not exist in the C++ class hierarchy; they are used here because they simplify S3 method dispatch.

Correct, and I expanded on the comments about that in the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants